Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs: Update Getting Started #3099

Merged
merged 2 commits into from
Jun 8, 2020

Conversation

yunhailuo
Copy link
Contributor

@yunhailuo yunhailuo commented May 25, 2020

Checklist:

  • This is a documentation bug fix.
  • The title of the PR is (a) conventional.
  • I've signed the CLA.
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My builds are green. Try syncing with master if they are not.
  • My organization is added to USERS.md.

@CLAassistant
Copy link

CLAassistant commented May 25, 2020

CLA assistant check
All committers have signed the CLA.

@yunhailuo
Copy link
Contributor Author

yunhailuo commented May 25, 2020

I was wondering if everything should just be in the default namespace but I'm not sure.

For beginners like me, it's a little distracting and potentially confusing for option like -n argo in a getting started. It always makes me think what is it; is it the key feature I'm trying to do with this command and the key feature I'm trying to learn? I won't necessarily miss the key but I'm not sure if that's really necessary.

I do agree, though, it's good to know argo CLI can use namespace like that. If it's not critical, I'm wondering if we could do -n default while installing the controller and/or with some notes just to tell and show people they can use namespace. For the rest of the tutorial, get the -n option out of people's way?

@alexec alexec self-assigned this May 25, 2020
@@ -102,7 +104,7 @@ helm install argo-artifacts stable/minio \

Login to the Minio UI using a web browser (port 9000) after exposing obtaining the external IP using `kubectl`.
```sh
kubectl get service argo-artifacts -o wide
kubectl get service argo-artifacts -o wide -n argo
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assumes the user has installed Minio into the Argo namespace, which isn't a safe assumption. Not that it's wrong, but we should add context here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree- I think the guide should probably be for installing into the argo namespace, but should state this at the start.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the comment @ntwrkguru . I'm trying to update the document to have everything installed into the argo namespace as stated at the very beginning. Just to clarify, where should this be clarified? Like

5. Install an Artifact Repository
...
Install Minio into the argo namespace:

?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a blurb, like:

 Assuming Minio was installed into the "argo" namespace, the following `kubectl` command will expose the Minio UI. Login to the Minio UI using a web browser (port 9000).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. Pushed one commit. I assume PRs will be squashed merged? Or should I squash myself?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexec Were there other "getting started" guides that need to be addressed? Ideally, there would be 1 guide that then could link into the examples or focus areas for more details.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not that really stand out. I'll follow the guide later and see if I can offer suggestions.

@sonarcloud
Copy link

sonarcloud bot commented May 26, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@alexec
Copy link
Contributor

alexec commented May 29, 2020

how do you peeps feel about this today?

@yunhailuo
Copy link
Contributor Author

how do you peeps feel about this today?

Not sure what you mean?

Not sure if @ntwrkguru tried it and has suggestions.

@ntwrkguru
Copy link
Contributor

LGTM. I didn't get a chance to run through, but we can always improve later.

@alexec alexec merged commit e26d2f0 into argoproj:master Jun 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants